src: squelch unused function warnings in util.h by solebox · Pull Request #9115 · nodejs/node

solebox · 2016-10-16T11:20:13Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passesd
commit message follows commit guidelines

Affected core subsystem(s)

src

Description of change

moved inline optimization to util-inl.h
Fixes: #9083

bnoordhuis · 2016-10-16T13:05:50Z

Thanks for the pull request. Looking at this, I think it may be better to move the definitions (i.e. the function bodies) of the non-template instances to util-inl.h and just leave declarations in util.h. WDYT?

solebox · 2016-10-16T21:06:08Z

maybe i should merge the files into one file called utils.h ?

bnoordhuis · 2016-10-17T09:38:17Z

src/util.h

You should leave declarations: inline char* Malloc(size_t);, etc.

changed declarations back to be inline as requested. (hope thats what you meant because here you referred to some definitions under the debated declarations by mistake so i got a little confused)

I think what @bnoordhuis would like to see is declarations like inline char* Malloc(size_t); in the util.h header, as that’s where people look to see what the actual internal APIs are.

bnoordhuis · 2016-10-17T09:39:51Z

maybe i should merge the files into one file called utils.h ?

The idea of two separate files is to speed up compilation for files that need the declarations but not the definitions.

solebox · 2016-10-17T11:19:30Z

added some commits, if you like the last commit (1 before last) and want me to squash it all into it then force push , tell me.

solebox · 2016-10-17T20:22:00Z

@addaleax done

addaleax

LGTM

(edit: with CI of course, I’d start one now but the interface doesn’t load…)

addaleax · 2016-10-18T13:03:26Z

src/util-inl.h

nit: Could you leave a blank line before this comment?

addaleax · 2016-10-18T13:07:33Z

src/util-inl.h

Does this change affect the warnings? This function doesn’t seem to be a particularly good candidate for inlining.

jasnell · 2016-10-19T15:51:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4578/

solebox · 2016-10-19T18:26:14Z

the build has failed but i cant access the CI URL

jasnell · 2016-10-20T16:52:57Z

Not sure what happened there. New CI: https://ci.nodejs.org/job/node-test-pull-request/4596/

solebox · 2016-10-25T10:35:34Z

@jasnell is it done? it has been runing for 5 days now...

jasnell · 2016-10-25T18:14:20Z

@bnoordhuis ... does this LGTY?

solebox · 2016-11-02T01:04:22Z

@addaleax @jasnell @bnoordhuis guys?

bnoordhuis

I'm afraid I can't start a test run, the CI is timing out again.

solebox · 2016-11-03T12:04:45Z

@jasnell can you help us out, please?

jasnell · 2016-11-03T15:11:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4772/

jasnell · 2016-11-03T15:12:26Z

Started another CI run. Will land after.

solebox · 2016-11-04T21:14:06Z

@Jansell looks like all the failing tests are simply timeouts
(and the flaky one)

jasnell · 2016-11-06T23:58:43Z

+1 ... will land this on Monday. Sorry for the delay, ended up getting side tracked on another item after starting the CI

solebox · 2016-11-08T20:51:43Z

@jasnell poke poke :)

jasnell · 2016-11-10T04:55:16Z

Sigh.. sorry, ended up completely buried all week. Lemme see if I can get this landed now

jasnell · 2016-11-10T05:03:01Z

Hmm... for some reason I am not able to land this even tho it says here that there are no conflicts. Git am is failing. Will investigate.

jasnell · 2016-11-10T05:06:48Z

@soleboxy ... can I ask you to please squash the commits down into a single commit, rebase and force push back?

solebox · 2016-11-10T13:26:12Z

@jasnell sure thing 👍

Fixes: nodejs#9083 moved function bodies back src: squelch unused function warnings in util.h. Fixes: nodejs#9083 moved back to previouse commit lets hope this is the correct answer src: squelch unused function warnings in util.h. Fixes: nodejs#9083 src: squelch unused function warnings in util.h. Fixes: nodejs#9083 src: squelch unused function warnings in util.h. Fixes: nodejs#9083 src: squelch unused function warnings in util.h. Fixes: nodejs#9083 src: squelch unused function warnings in util.h. Fixes: nodejs#9083

solebox · 2016-11-14T15:29:51Z

@jasnell pong

bnoordhuis · 2016-11-16T18:02:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4860/

Fixes: #9083 PR-URL: #9115 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell · 2016-11-18T16:32:23Z

Landed in 35d48d3! (finally ;-) ...) Sorry for the delay!

Fixes: #9083 PR-URL: #9115 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2016-12-13T21:05:02Z

should this be backported?

bnoordhuis · 2016-12-13T21:27:58Z

Only if it applies cleanly. It's a cosmetic change.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 16, 2016

solebox changed the title ~~src: squelch unused function warnings in util.h (moved inline optimization to util-inl.h)~~ src: squelch unused function warnings in util.h Oct 16, 2016

vitkarpov mentioned this pull request Oct 16, 2016

src: squelch unused function warnings in util.h #9083

Closed

solebox force-pushed the src_squelch_unused_function_warnings branch from ff1c6da to 90fb363 Compare October 16, 2016 21:43

bnoordhuis requested changes Oct 17, 2016

View reviewed changes

solebox force-pushed the src_squelch_unused_function_warnings branch from a3e751f to 00127bb Compare October 17, 2016 10:39

addaleax added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 18, 2016

addaleax approved these changes Oct 18, 2016

View reviewed changes

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

jasnell approved these changes Oct 19, 2016

View reviewed changes

bnoordhuis approved these changes Nov 2, 2016

View reviewed changes

jasnell self-assigned this Nov 3, 2016

solebox force-pushed the src_squelch_unused_function_warnings branch from 2cf933b to 57a762f Compare November 10, 2016 13:29

jasnell closed this Nov 18, 2016

MylesBorins added lts-watch-v4.x labels Dec 13, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Dec 21, 2016

aliscco mentioned this pull request Nov 29, 2023

[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#705

Open

leonardoadame mentioned this pull request Nov 29, 2023

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#246

Open

This was referenced Nov 30, 2023

[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#826

Open

[Snyk] Fix for 3 vulnerabilities aliscco/alisco-node#846

Open

Uh oh!

Conversation

solebox commented Oct 16, 2016 • edited by Fishrock123 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

bnoordhuis commented Oct 16, 2016

Uh oh!

solebox commented Oct 16, 2016

Uh oh!

bnoordhuis Oct 17, 2016

Choose a reason for hiding this comment

Uh oh!

solebox Oct 17, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 17, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 17, 2016

Uh oh!

solebox commented Oct 17, 2016

Uh oh!

solebox commented Oct 17, 2016

Uh oh!

addaleax left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 19, 2016

Uh oh!

solebox commented Oct 19, 2016

Uh oh!

jasnell commented Oct 20, 2016

Uh oh!

solebox commented Oct 25, 2016

Uh oh!

jasnell commented Oct 25, 2016

Uh oh!

solebox commented Nov 2, 2016

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

solebox commented Nov 3, 2016

Uh oh!

jasnell commented Nov 3, 2016

Uh oh!

jasnell commented Nov 3, 2016

Uh oh!

solebox commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Nov 6, 2016

Uh oh!

solebox commented Nov 8, 2016

Uh oh!

jasnell commented Nov 10, 2016

Uh oh!

jasnell commented Nov 10, 2016

Uh oh!

jasnell commented Nov 10, 2016

Uh oh!

solebox commented Nov 10, 2016

Uh oh!

solebox commented Nov 14, 2016

Uh oh!

bnoordhuis commented Nov 16, 2016

Uh oh!

jasnell commented Nov 18, 2016

Uh oh!

MylesBorins commented Dec 13, 2016

solebox commented Oct 16, 2016 •

edited by Fishrock123

Loading

addaleax left a comment •

edited

Loading

solebox commented Nov 4, 2016 •

edited

Loading